Skip to content

[SYSTEMDS-3953] Switch quantile default to R quantile type 7 - #2585

Open
MegaByteTron wants to merge 4 commits into
apache:mainfrom
MegaByteTron:SYSTEMDS-3953-quantile-type7
Open

[SYSTEMDS-3953] Switch quantile default to R quantile type 7#2585
MegaByteTron wants to merge 4 commits into
apache:mainfrom
MegaByteTron:SYSTEMDS-3953-quantile-type7

Conversation

@MegaByteTron

Copy link
Copy Markdown
Contributor

Summary

Fixes both issues reported in SYSTEMDS-3953:
scalar p vs matrix p returning different answers because the
average flag was not propagated from pickValues to pickValue, and
even-n inputs averaging the two order statistics straddling ceil(p·n)
— which matches R type 7 only at p = 0.5 and matches nothing named in
Hyndman & Fan (1996) elsewhere.

Switches the SystemDS default to R quantile type 7 across the kernel,
CP, SP, and FED paths. Supersedes PR #2497
([SYSTEMDS-3922]/[SYSTEMDS-3898]) which patched averaging on even n only
for p = 0.5. Continues PR #2565
by @ywcb00 which added the failing-tests-as-@Ignore scaffolding
(d6aabf6867); those tests are un-ignored here.

IQM is intentionally preserved — it is a trimmed weighted mean, not an
order-statistic pick, and does not fit the type-7 interpolation shape.
IQMTest is the guardrail.

R type 7 (1-indexed, n = |x|):

h  = (n − 1) · p + 1
lo = floor(h)                 // clamped to [1, n]
hi = min(lo + 1, n)
g  = h − lo                   // in [0, 1)
Q  = (1 − g) · x[lo] + g · x[hi]

Ticket example — [0.239, 0.517, 0.890, 0.944] at p = 0.25, 0.5, 0.75
now returns 0.4475, 0.7035, 0.9035, matching R.

Commit 1 — [SYSTEMDS-3953] Switch quantile kernel to R quantile type 7

MatrixBlock.computeType7Rank(long n, double p) is the new static
helper returning {lo, hi, g} — one primitive every consumer picks
against so the formula lives in one place. pickUnweightedValue /
pickWeightedValue / pickValues / median rewired to type 7;
pickValue(double) becomes the public API and the pickValue(double, boolean average) overload is deleted. QuantilePickCPInstruction
drops the matBlock.getLength() % 2 == 0 argument. QuantilePickTest
expected values updated to type 7; CompressedSortTest drops the
now-dead pickValue(q, true) averaging assertion.

Commit 2 — [SYSTEMDS-3953] Rework Spark quantile pick to R type 7

processInstruction becomes a clean switch: VALUEPICK / MEDIAN →
pickQuantileValues, IQM → new private computeIqm. The pre-3953
shape mixed both under a getWeightedQuantileSummary blob whose fields
meant different things per operation. pickQuantileValues handles
N ≥ 1 quantiles uniformly; weighted branch pulls (lo, hi, g) triples
from a new shared extractWeightedTriples helper. Type7Rank inner
class and getWeightedQuantileSummary removed.

Un-ignore testQuartileArray{CP,SP} (marked FIXME: fix SYSTEMDS-3953
by David in d6aabf6867) and add testQuantileEven{1,2,3}{CP,SP} at
n = 128 — the odd-length default (n = 1973) can't exercise g ≠ 0
interpolation because at classical p the rank lands on an integer.

Commit 3 — [SYSTEMDS-3953] Switch federated quantile pick to R type 7

processRowQPick computes per-quantile rank triples (lo, hi, g) up
front, flattens to a deduplicated int[] of ranks, feeds those to
pickMultipleRanks (renamed from computeMultipleQuantiles to signal
its narrowed job), and interpolates at the caller. Decoupling ranks
from interpolation keeps the multi-rank pipeline type-7-agnostic.
Consequence: the boolean average thread across four signatures
(processRowQPick, createHistogram, getBucketWithIndex,
getSingleQuantileResult) is removed end-to-end. computeIqm mirrors
the SP helper from commit 2 so the two paths read symmetrically.
refineBucket shares the coarse-histogram refinement heuristic between
computeIqm and pickMultipleRanks. MEDIAN in processColumnQPick
dispatches through VALUEPICK with p = 0.5; the ColMedian inner UDF
goes away.

Weighted row-federated inherits the kernel's sum-of-weights extension
(N = Σw) for free. Column-federated weighted stays out of scope,
matching PR #2497.

quantile(x, p) in SystemDS had two bugs at the kernel layer:

  1. Scalar p vs matrix p returned different answers for the same input
     because the average flag was not propagated from pickValues to
     pickValue.
  2. Even n averaged the two order statistics straddling ceil(p*n),
     which matches R type 7 only at p = 0.5 and matches nothing named
     in Hyndman and Fan 1996 elsewhere.

Switch the default to R quantile type 7 (h = (n-1)*p + 1, interpolate
between the two adjacent order statistics with weight g = h - floor(h)).
For the ticket example [0.239, 0.517, 0.890, 0.944] at p = 0.25, 0.5,
0.75 the kernel now returns 0.4475, 0.7035, 0.9035 — matching R.

MatrixBlock:

  * computeType7Rank(long n, double p) is the new static helper — returns
    {lo, hi, g} as double[3]. Sits next to computeIQMCorrection with the
    same shape template. Every consumer (kernel, CP, SP, FED) picks
    against this one primitive so the formula lives in exactly one place.
  * pickUnweightedValue(double) picks against getNumRows() with type 7.
  * pickWeightedValue(double) picks against Math.round(sumWeightForQuantile())
    with the same formula, treating the two-column weighted input as an
    expanded sorted sequence of length sumWeights.
  * pickValue(double) is the new public API; the pickValue(double,
    boolean average) overload is removed (dead switch — type 7 is
    unconditional at the kernel layer).
  * pickValues(qs, ret) drops the average flag.
  * median() collapses to pickValue(0.5).
  * interQuartileMean and computeIQMCorrection are untouched — IQM is a
    trimmed weighted mean, a different statistic that does not fit the
    type-7 interpolation shape.

CP: QuantilePickCPInstruction drops the matBlock.getLength() % 2 == 0
argument to pickValue and pickValues. median() call unchanged.

Tests:

  * QuantilePickTest expected values updated to type-7 (odd-n cases
    unchanged; even-n cases now interpolate).
  * CompressedSortTest drops the pickValue(q, true) averaging line —
    with the boolean overload gone, one assertion covers the new
    unconditional-type-7 contract.

SP, FED, and integration tests come in follow-up commits on this branch.
Bring the Spark path in line with the kernel switch to R quantile type 7
in the previous commit. Row-partitioned single-column and weighted
two-column inputs both return type-7 interpolated values matching CP.

QuantilePickSPInstruction:

  * processInstruction reads as a clean switch — VALUEPICK / MEDIAN go
    through pickQuantileValues, IQM goes through the extracted
    computeIqm. The pre-3953 shape mixed the two under a single
    getWeightedQuantileSummary blob whose fields meant different things
    per operation; the split makes each case's data shape explicit.
  * pickQuantileValues handles N >= 1 quantiles uniformly. Weighted
    branch stores rank g's in a primitive double[] and pulls (lo, hi, g)
    triples from extractWeightedTriples; unweighted branch uses
    MatrixBlock.computeType7Rank and interpolates from the collected
    sorted values.
  * computeIqm is the extracted IQM path — reuses extractWeightedTriples
    but combines with computeIQMCorrection, not type 7. IQM behavior is
    numerically unchanged from pre-3953.
  * extractWeightedTriples is the shared helper. Locates the partition
    holding each ceil-based key by scanning cumulative per-partition
    weights, then invokes ExtractWeightedQuantileFunction to fetch the
    (position, posPart, value) triples.
  * Type7Rank inner class and getWeightedQuantileSummary are both
    removed; the static kernel helper subsumes the former, and the
    per-consumer split obsoletes the latter.

Tests: un-ignore testQuartileArrayCP / testQuartileArraySP (marked with
FIXME: fix SYSTEMDS-3953 by David); add QuantileEven{1,2,3}{CP,SP} at
n = 128 for p in {0.25, 0.5, 0.75}. Odd-n cases (rows = 1973) can't
exercise interpolation because the rank lands on an integer at classical
p; the even-n additions cover the g != 0 path in both CP and SP.

FED path in the follow-up commit on this branch.
Bring the FED path in line with the CP/SP switch to R quantile type 7 in
the previous two commits. Row-federated matrices now return the same
type-7 interpolated values as CP: [0.239, 0.517, 0.890, 0.944] at
p = 0.25, 0.5, 0.75 gives 0.4475, 0.7035, 0.9035.

processRowQPick's VALUEPICK / MEDIAN branch computes per-quantile rank
triples (lo, hi, g) via MatrixBlock.computeType7Rank, flattens to a
deduplicated int[] of ranks, feeds those to pickMultipleRanks (renamed
from computeMultipleQuantiles to signal it now does one job: fetch the
value at each rank), and interpolates (1 - g) * v_lo + g * v_hi at the
caller. Decoupling ranks from interpolation lets the multi-rank pipeline
stay type-7-agnostic — an earlier attempt threading an isType7 flag
through createHistogram / getBucketWithIndex regressed VALUEPICK to
type 1 and mis-computed q25 boundaries, so this shape avoids
reintroducing that seam.

Consequence of the type-7 unification: the boolean average thread across
processRowQPick, createHistogram, getBucketWithIndex, and
getSingleQuantileResult (which only ever handled the even-n median
average case) is removed end-to-end.

IQM stays a raw ceil-based trimmed weighted mean and is extracted into
private computeIqm, mirroring the SP computeIqm helper from the previous
commit so processRowQPick reads as VALUEPICK/MEDIAN -> rank pipeline,
IQM -> computeIqm — symmetric with SP.

refineBucket wraps the "recurse into a coarse-histogram bucket with a
finer sub-histogram" step so computeIqm and pickMultipleRanks share the
nextNumBuckets heuristic in one place instead of two.

MEDIAN in processColumnQPick now dispatches through the VALUEPICK path
with p = 0.5 (kernel pickValue(0.5) is already type 7 after the first
commit), which lets the ColMedian inner UDF go away. VALUEPICK and its
ValuePick UDF are otherwise unchanged.

Weighted row-federated inherits the kernel's sum-of-weights extension
for free: N = sumWeights in the two-column case flows through the same
rank formula. Column-federated weighted stays out of scope, matching
PR apache#2497.

Verified: FederatedQuantileTest 24/24, FederatedQuantileWeightsTest
12/12, QuantileTest 32/32 (including newly un-ignored
testQuartileArray{CP,SP} and the QuantileEven cases), IQMTest green,
QuantilePickTest green.
@ywcb00

ywcb00 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Thank you @MegaByteTron. :)
I ran the CI workflows and five workflows failed. Could you please have another look at this?
All the best, David

The default-quantile flip surfaced two problems on the branch:

1. QuantilePickFEDInstruction#getEquiHeightBins was double-scaling its
   input. The caller (MultiReturnParameterizedBuiltinFEDInstruction)
   hands it raw 1-based ranks in [1..N] (numRows/numBin * (i+1)), not
   [0,1] probabilities, so multiplying by N again pushed every rank
   past the histogram, left bucketsWithIndex[i] null, and blew up
   TransformFederatedEncodeApplyTest with an NPE on refineBucket.middle.
   Use the values as ranks directly, clamped to [1..N].

2. Three R-comparison scripts and two hand-rolled helpers still encoded
   the old sort[ceil(p*n)] rule and disagreed with the new type-7 DML:
     - Scale.R, WeightedScaleTest.R, scaleRobust.R: quantile(type=1) -> type=7
     - OrderStatisticsTest#quantile: switched to R type 7 interpolation
     - test_quantile.py weighted_quantiles: rewrote to interpolate the
       weighted (lo, hi) rank pair instead of picking a single rank
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants